fix(db): compose optimistic updates from changes instead of whole-row snapshots - #1701
fix(db): compose optimistic updates from changes instead of whole-row snapshots#1701marbemac wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOptimistic updates now merge only changed fields over the current row, preserving unrelated fields across concurrent transactions, sync commits, and rollbacks. Tests cover composition, defaults, synchronization, rollback isolation, and redundant event suppression. ChangesOptimistic composition
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MILLERMARRU
left a comment
There was a problem hiding this comment.
I checked collection/state.ts and confirmed both places that seed optimisticUpserts for an update mutation just did mutation.modified as TOutput, a full snapshot, not a diff. That's the exact bug: two transactions mutating different fields on the same row would fight over which snapshot wins the overlay, and rolling back one would blow away fields a sibling transaction owns, since there's no per-field ownership tracked anywhere. resolveOptimisticUpsert composing {...base, ...mutation.changes} fixes this cleanly. The caching by the changes object reference plus a base === cached.base check is a nice detail, it keeps object identity stable for an unchanged overlay so downstream consumers relying on reference equality (e.g. to suppress redundant update events) don't get spurious churn, which the last test in the suite explicitly checks. I'd double check the WeakMap doesn't leak in long-lived collections with high mutation churn, but since it's keyed by the changes object (per-mutation and short-lived), that seems fine.
🎯 Changes
Small fix to how the optimistic overlay composes rows.
Today an optimistic insert/update stores
mutation.modified— the whole row as it looked atmutate()time — intooptimisticUpserts. Sincemodifiedis built from the visible row (Object.assign({}, visibleRow, payload)inmutations.ts), whichever transaction sorts last dictates every field, including ones it never touched.The clearest symptom: rolling back one of several in-flight transactions doesn't actually remove its change.
It doesn't change when committed sync gets applied. Sync still stays queued while a transaction is
persisting, exactly as today. That's the half of #1630 that turned out to be unsound without temp/server key mapping, and it's not needed for any of this.So this is only the projection half — same base as before, just composed per-field instead of replaced wholesale.
Related to RFC #1625.
t3's snapshot was taken when
bwas alreadyb2, so once t2 is gone t3 puts it right back. I don't think there's a reading of that which is correct.The fix:
updatemutations now composemutation.changesover the value they were layered on, so a mutation only owns the fields it actually changed. Inserts keep usingmodified, since an insert'schangesdeliberately omit schema defaults (there's a comment inmutations.tssaying as much).✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit